Norm Samples updates for B,S,H style tensor inputs#432
Open
rmhaskarnvidia wants to merge 5 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughLayerNorm and RMSNorm tensor layouts changed from flattened 4D forms to explicit sequence-oriented shapes. Backward property inference now copies dimensions and strides from input and scale tensors, with corresponding C++ samples, Python notebooks, and tests updated. ChangesNormalization layout updates
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
samples/python/29_rmsnorm.ipynb (1)
318-318: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRun
black-jupyteron the modified notebook.The trailing semicolons are not Black-formatted. As per coding guidelines, Python tutorial notebooks must use
black-jupyterformatting.Also applies to: 436-436
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@samples/python/29_rmsnorm.ipynb` at line 318, Run black-jupyter on the modified notebook, including the cells containing inv_var.set_name(...) and the corresponding code at the other referenced location, so Black removes the trailing semicolons and applies standard formatting throughout the notebook.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@samples/python/29_rmsnorm.ipynb`:
- Line 18: Update the Open in Colab badge URL in the notebook’s introductory
markdown cell to reference 29_rmsnorm.ipynb instead of 28_rmsnorm.ipynb, leaving
the badge text and repository path unchanged.
- Around line 338-345: Update the Python Binding API setup before the
variant_pack definition to allocate local out_gpu and inv_var_gpu tensors,
rather than relying on variables created by the earlier wrapper flow. Ensure the
existing out and inv_var bindings continue to reference these locally defined
outputs when running the flow independently.
In `@test/python/test_layernorm.py`:
- Line 58: Update every reference torch.var call used by the LayerNorm
expected-value calculations to pass correction=0: test/python/test_layernorm.py
lines 58-58, samples/python/20_layernorm_forward.ipynb lines 168-170 and
258-260, and samples/python/21_layernorm_backward.ipynb lines 174-176 and
337-339. Preserve the existing dimensions and keepdim arguments.
---
Nitpick comments:
In `@samples/python/29_rmsnorm.ipynb`:
- Line 318: Run black-jupyter on the modified notebook, including the cells
containing inv_var.set_name(...) and the corresponding code at the other
referenced location, so Black removes the trailing semicolons and applies
standard formatting throughout the notebook.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3b1aa3cf-7be5-4702-9738-7390a6624071
📒 Files selected for processing (11)
include/cudnn_frontend/node/dln.hinclude/cudnn_frontend/node/rmsnorm.hsamples/cpp/norm/batchnorm.cppsamples/cpp/norm/layernorm.cppsamples/cpp/norm/rmsnorm.cppsamples/python/20_layernorm_forward.ipynbsamples/python/21_layernorm_backward.ipynbsamples/python/22_layernorm_inference.ipynbsamples/python/29_rmsnorm.ipynbtest/python/test_layernorm.pytest/python/test_rmsnorm.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before submitting
pre-commit runand committed any formatting changes.Affected area
Documentation or samples
Summary
Layout and RMS Norm samples are updated to accept direct B,S,H tensor shapes
Why
cudnn Backend supports passing X tensor as (B,S,H) for LN/RMS norm. All FE samples transformed the input to (B*S, H ,1,1) which is not really necessary
API and compatibility impact
None
Testing
./bin/samples "LayerNorm Backward"
Filters: "LayerNorm Backward"
Randomness seeded to: 1937091254
All tests passed (7 assertions in 1 test case)
./bin/samples "LayerNorm Inference"
Filters: "LayerNorm Inference"
Randomness seeded to: 3002993422
All tests passed (9 assertions in 1 test case)
./bin/samples "LayerNorm Training"
Filters: "LayerNorm Training"
Randomness seeded to: 4011977597
All tests passed (7 assertions in 1 test case)
/bin/samples "RmsNorm Backward"
Filters: "RmsNorm Backward"
Randomness seeded to: 572308681
All tests passed (8 assertions in 1 test case)
./bin/samples "RmsNorm Inference"
Filters: "RmsNorm Inference"
Randomness seeded to: 4203307452
All tests passed (8 assertions in 1 test case)
Summary by CodeRabbit
DY/DXwithXand scale-related tensors withSCALEwhen not explicitly set.(batch, sequence, hidden)layouts (including corresponding scale/bias and updated mean/inv-variance handling).